.github/workflows/build.yml (1)

59-59: Same verification needed as in check-pr.yml.

This change mirrors the one in .github/workflows/check-pr.yml. Please ensure the test output structure is consistent with this specific file path.

src/ghosttext-adaptor/email_editor.ts (1)

29-32: Consider filtering out false values for clarity.

The current implementation works correctly because Promise.all handles non-Promise values, but the pattern isNewStatus && promise can be confusing since it evaluates to either false or a Promise.

For better readability, consider filtering out the false value:

-    await Promise.all([
-      isNewStatus && this.notificationTray.showNotification(icon, messageForStatus(status)),
-      this.composeWindow.setIcon(icon),
-    ])
+    const tasks = [this.composeWindow.setIcon(icon)]
+    if (isNewStatus) {
+      tasks.push(this.notificationTray.showNotification(icon, messageForStatus(status)))
+    }
+    await Promise.all(tasks)

Alternatively, use filter:

-    await Promise.all([
-      isNewStatus && this.notificationTray.showNotification(icon, messageForStatus(status)),
-      this.composeWindow.setIcon(icon),
-    ])
+    await Promise.all(
+      [
+        isNewStatus && this.notificationTray.showNotification(icon, messageForStatus(status)),
+        this.composeWindow.setIcon(icon),
+      ].filter(Boolean)
+    )

src/thunderbird/messenger/i18n.ts (1)

3-20: Clarify documentation to match the simplified signature.

The documentation mentions:

• Returning undefined for format errors, but the signature only allows string
• A substitutions parameter that doesn't exist in the signature

Since this interface appears to be a simplified version of browser.i18n.getMessage(), update the documentation to reflect that this implementation:

• Only accepts messageName (no substitutions support)
• Only returns string (clarify what happens on errors)

Apply this diff to align documentation with the actual signature:

   /**
    * Gets the localized string for the specified message. If the message is
-   * missing, this method returns an empty string (''). If the format of
-   * the `getMessage()` call is wrong — for example, _messageName_ is not a
-   * string or the _substitutions_ array has more than 9 elements — this
-   * method returns `undefined`.
+   * missing, this method returns an empty string ('').
    *
    * @param messageName The name of the message, as specified in the
    * `messages.json` file.
    *
-   * @param [substitutions] Substitution strings, if the message requires
-   * any.
-   *
    * @returns Message localized for current locale.
    */
   getMessage(messageName: MessageId): string

src/app-background/menu_handler.ts (1)

27-32: Add validation before casting menuItemId to CommandId.

The type assertion on line 30 assumes menuItemId is always a valid CommandId, but there's no runtime validation. If an invalid ID is passed (e.g., from a stale menu or external source), CommandHandler.runCommand will fail at runtime.

Consider adding validation:

   /** Handles click on a menu item */
   handleMenuItemClicked(menuItemId: string, composeTab: IComposeWindow): Promise<void> {
     // We use command ID as menu item ID, so we can directly pass it to the command handler
+    const validCommands: CommandId[] = ["start_ghostbird", "stop_ghostbird", "toggle_ghostbird"]
+    if (!validCommands.includes(menuItemId as CommandId)) {
+      return Promise.reject(new Error(`Invalid menu item ID: ${menuItemId}`))
+    }
     return this.commandHandler.runCommand(menuItemId as CommandId, composeTab)
   }

Alternatively, create a type guard function:

function isCommandId(id: string): id is CommandId {
  return ["start_ghostbird", "stop_ghostbird", "toggle_ghostbird"].includes(id)
}

src/root/background.ts (1)

53-59: Remove unsafe assertion and add a type guard
Replace the cast (info as MenuShownInfo) with a runtime check to ensure info.menuIds is an array before passing it to handleMenuShown. For example:

if (!Array.isArray(info.menuIds)) {
  throw new TypeError('Unexpected menu info shape');
}
return router.handleMenuShown(info, tab);

src/thunderbird/background_util/button_menu.ts (3)

39-41: Avoid global menus.removeAll() if you have other context menus.

This removes all of this extension’s menu items across contexts. Prefer tracking created IDs and removing only those, or only calling this once at startup.

---

31-36: Remove noisy debug logging (or gate it).

console.debug(got) on every load is chatty. Consider removing or gating behind a verbose flag.

Suggested change:

-    console.debug(got)

---

17-24: Optionally validate current menu vs. expected items before early-return.

You accept currentShown but ignore it. To self-heal if menus get cleared, compare info.menuIds with expected and re-init when mismatched.

src/app-background/background_event_router.ts (1)

16-23: Validate command before casting to CommandId.

Unsafe cast can pass an unknown string to runCommand, which then returns undefined (not a Promise). Fail fast on unknown commands.

Apply this diff:

   handleCommand(command: string, tab: ITab): Promise<void> {
     let composeTab = this.composeTabDetector.tryWrap(tab)
     if (!composeTab) {
       return Promise.reject(Error("Event dropped"))
     }
 
-    return this.commandHandler.runCommand(command as CommandId, composeTab)
+    const allowed = new Set<CommandId>(["start_ghostbird", "stop_ghostbird", "toggle_ghostbird"])
+    if (!allowed.has(command as CommandId)) {
+      return Promise.reject(Error(`Unknown command: ${command}`))
+    }
+    return this.commandHandler.runCommand(command as CommandId, composeTab)
   }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/thunderbird/background_util/button_menu.ts (1)

17-24: Concurrent initialization concern.

This was flagged in a previous review: multiple rapid onShown events could potentially cause race conditions leading to duplicate menus.create calls and "duplicate id" errors. While the ??= operator provides some protection, the suggested explicit serialization pattern would be more robust.

Additionally, lines 18-20 perform a check but don't return early, making them redundant with the ??= operator on line 21. Consider either:

• Removing the check entirely (let ??= handle it), or
• Adding an early return after the log for efficiency:

  initItems(menuItems: ReadonlyArray<MenuItem>, _currentShown: MenuShownInfo | undefined): Promise<void> {
    if (this.initWork) {
      console.debug("ButtonMenu is already initialized; Skip creating menu items.")
+     return this.initWork
    }
    this.initWork ??= this.createMenuItems(menuItems)

    return this.initWork
  }

src/app-background/menu_handler.ts (2)

15-25: Implement the comparison hinted by the comment.

Re-init when current menu differs from expected item IDs.

   handleMenuShown(info: MenuShownInfo): Promise<void> | void {
-    // Compare the shown menu with menuItems and (re-)initialize the menu if necessary
-    console.debug(info)
-
-    if (!this.buttonMenu.isInitialized()) {
+    // (Re-)initialize if uninitialized or IDs differ
+    console.debug(info)
+    if (!this.buttonMenu.isInitialized()) {
       console.debug("Initializing menu")
       return this.buttonMenu.initItems(this.menuItems, info)
     }
 
-    console.debug("Menu is already initialized")
+    const expected = new Set(this.menuItems.map((m) => m.id))
+    const actual = new Set(info.menuIds)
+    const same =
+      expected.size === actual.size && [...expected].every((id) => actual.has(id))
+    if (!same) {
+      console.debug("Menu items changed; re-initializing menu")
+      return this.buttonMenu.initItems(this.menuItems, info)
+    }
+
+    console.debug("Menu is already initialized")
   }

---

27-31: Defensive guard on menuItemId before casting.

Avoid relying on casts; reject unknown IDs early.

-  handleMenuItemClicked(menuItemId: string, composeTab: IComposeWindow): Promise<void> {
-    // We use command ID as menu item ID, so we can directly pass it to the command handler
-    return this.commandHandler.runCommand(menuItemId as CommandId, composeTab)
-  }
+  handleMenuItemClicked(menuItemId: string, composeTab: IComposeWindow): Promise<void> {
+    // Validate the ID before delegating
+    const isCommandId = (id: string): id is CommandId =>
+      id === "start_ghostbird" || id === "stop_ghostbird" || id === "toggle_ghostbird"
+    if (!isCommandId(menuItemId)) {
+      return Promise.reject(Error(`unknown command ${menuItemId}`))
+    }
+    return this.commandHandler.runCommand(menuItemId, composeTab)
+  }

src/app-background/background_event_router.ts (2)

22-22: Validate command string before casting.

Reject unknown commands here to avoid undefined bubbling.

-    return this.commandHandler.runCommand(command as CommandId, composeTab)
+    if (!isCommandId(command)) {
+      return Promise.reject(Error(`unknown command ${command}`))
+    }
+    return this.commandHandler.runCommand(command, composeTab)

Add this helper in the module (outside the class):

const isCommandId = (id: string): id is CommandId =>
  id === "start_ghostbird" || id === "stop_ghostbird" || id === "toggle_ghostbird"

---

43-50: Also guard menuItemId here.

Mirror the command guard for menu clicks.

-    return this.menuHandler.handleMenuItemClicked(menuItemId, composeTab)
+    if (!isCommandId(menuItemId)) {
+      return Promise.reject(Error(`unknown command ${menuItemId}`))
+    }
+    return this.menuHandler.handleMenuItemClicked(menuItemId, composeTab)

src/thunderbird/background_util/button_menu.ts (1)

13-15: Consider clarifying initialization state semantics.

The method returns true when initialization has started, not when it has completed. While this matches the interface contract, it could be semantically confusing. Consider renaming to hasInitializationStarted() or documenting this behavior more explicitly.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: snipsnipsnip
PR: exteditor/ghostbird#39
File: src/thunderbird/background_util/button_menu.ts:47-66
Timestamp: 2025-10-15T13:30:46.981Z
Learning: `Promise.withResolvers` is supported in TB128/Gecko runtime used by the Thunderbird extension and is already used in multiple files like `src/util/promisifying_queue.ts` and `src/util/lazy_then.ts`.

Learnt from: snipsnipsnip
PR: exteditor/ghostbird#39
File: src/thunderbird/background_util/button_menu.ts:52-53
Timestamp: 2025-10-15T14:05:25.989Z
Learning: In Thunderbird WebExtension API, the `menus.create` method's `icons` parameter accepts either a string (MenuIconPath) or an object dictionary (MenuIconDictionary), unlike the standard WebExtension API which only accepts an object. A string can be passed directly as the value for the icons property.

src/root/background.ts (2)

16-23: LGTM!

The menu configuration is correct. The i18n key "manifest_commands_stop_ghostbird_description" is properly defined in /locales.toml (per project conventions), and the icon path "gray.svg" aligns with existing assets.

Based on learnings.

---

61-71: LGTM!

The validation logic correctly filters out events from special windows (when tab is falsy) and ensures menuItemId is a string before delegating to the router. The error handling appropriately rejects unknown commands.

Based on learnings.

src/app-background/menu_handler.ts (1)

28-31: Validate menuItemId before casting to CommandId.

The type assertion menuItemId as CommandId on line 30 is unsafe. If a caller passes an invalid menuItemId, the cast will succeed but commandHandler.runCommand will fail with a type error at runtime (the switch statement has no default case).

Consider adding validation:

  handleMenuItemClicked(menuItemId: string, composeTab: IComposeWindow): Promise<void> {
-   // We use command ID as menu item ID, so we can directly pass it to the command handler
-   return this.commandHandler.runCommand(menuItemId as CommandId, composeTab)
+   // We use command ID as menu item ID, so we can directly pass it to the command handler
+   const validCommands: ReadonlyArray<CommandId> = ["start_ghostbird", "stop_ghostbird", "toggle_ghostbird"]
+   if (!validCommands.includes(menuItemId as CommandId)) {
+     return Promise.reject(Error(`Invalid menu item ID: ${menuItemId}`))
+   }
+   return this.commandHandler.runCommand(menuItemId as CommandId, composeTab)
  }

Alternatively, if the upstream validation in src/root/background.ts (line 64) already ensures menuItemId is a string, consider documenting this contract or adding a runtime assertion.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: snipsnipsnip
PR: exteditor/ghostbird#39
File: src/root/background.ts:61-71
Timestamp: 2025-10-16T08:50:50.310Z
Learning: In Thunderbird WebExtensions, when `messenger.menus.onClicked` fires with a falsy `tab` argument, it indicates the event originated from a special window (such as Thunderbird's debug tools) rather than a regular compose window. In `src/root/background.ts`, the intentional design is to reject such events by checking `if (!tab)` to filter out these non-compose-window events.

Learnt from: snipsnipsnip
PR: exteditor/ghostbird#39
File: src/root/background.ts:16-23
Timestamp: 2025-10-16T08:52:27.564Z
Learning: In the ghostbird project, i18n keys are defined in `/locales.toml` and then generated into `_locales/*/messages.json` files using the build script `tools/generate_locale_messages.ts`. When reviewing i18n key usage, check `/locales.toml` rather than the generated `messages.json` files.